fix #155 #161
fix #155 #161
Conversation
silkentrance
force-pushed
the
gh-155
branch
3 times, most recently
from
d1b72a5 to
8db6ad1
Compare
|
@raszi are you ok with adding the rimraf dependency and eliminating our own code? as otherwise, we would have to reimplement rimraf for async operation. If so, I would then proceed and merge this to master ASAP, after the cleanup of course. |
silkentrance
force-pushed
the
gh-155
branch
2 times, most recently
from
8f665db to
7c51078
Compare
|
Well, I would have no problems introducing a small dependency for the job and removing our code. My concerns with rimraf are the followings:
|
|
@raszi Sorry for not coming back to you and working on this for quite a while now. I will see whether there is a different approach to that. |
|
@raszi the glob package is also used by npm itself, and, according to npm stats, it has a whopping 15mil (not US billion) downloads. In fact, it is a well proven dependency. As to the failing tests, since we eliminated node < 0.10.0 the tests are no longer failing. |
silkentrance
force-pushed
the
gh-155
branch
2 times, most recently
from
15305b8 to
7d25c8c
Compare
|
@raszi I fixed the order by which the tests cleanup their remnant temporary files. Sometimes, especially with node 0.10.x this would fail and done() would not be called. |
|
rebased to master |
|
Can this PR be merged? |
|
@kaiserfro From my part yes, but @raszi had some reservations towards the introduction of the dependency towards the glob package. |
|
@kaiserfro since this integrates the latest changes from master, you could use a github reference for your dependency, unless you do not want to do that. |
fix order by which tests rimraf their leftovers
|
@raszi since I have not heard back from you for a long time and since people actually need this functionality, I will continue with merging this into master as soon as all tests have succeeded. |
| throw e; | ||
| } | ||
| // reraise any unanticipated error | ||
| if (!isENOENT(e)) throw e; |
There was a problem hiding this comment.
This will cause a linter error because of no-unsafe-finally.
| next(); | ||
| } | ||
|
|
||
| if (0 <= fdPath[0]) |
There was a problem hiding this comment.
This is too dense and error prone. Please put parentheses around the branches.
| if (!opts.keep) { | ||
| _removeObjects.unshift(removeCallback); | ||
| } | ||
| const removeFunction = opts.unsafeCleanup ? _rimrafRemoveDirWrapper : fs.rmdir.bind(fs); |
There was a problem hiding this comment.
This is technically calling rimraf. Why do we need a wrapper for this?
| * Simple wrapper for rimraf.sync. | ||
| * | ||
| * @param {string} dirPath | ||
| * @private |
There was a problem hiding this comment.
The next parameter is not documented.
| @@ -3,6 +3,7 @@ | |||
| environment: | |||
| matrix: | |||
| - nodejs_version: "6" | |||
| - nodejs_version: "7" | |||
|
|
||
| if (0 <= fdPath[0]) | ||
| fs.close(fdPath[0], function (err) { | ||
| fs.unlink(fdPath[1], _handler); |
There was a problem hiding this comment.
I believe we should extract this to a local function because it is also used in the else branch.
const _removeFile = function () { fs.unlink(fdPath[1], _handler); }
|
Sorry for the delay, this is a super busy period of my life. |
|
@raszi I am currently also rather busy. Thanks for the review. I will try to fix these issues ASAP. |
With this in place, the user will be handed out an async remove callback, as requested in #155. Internally, we will be using the sync remove callback so that on process exit, everything is done synchronously.
This also fixes existing async tests, at last.
The problem with the existing async test code was, erm, that assertions had been made outside of a try catch statement, leading to tests that seemed to work just fine, however, done() was never called in case of error.
😕 still wondering why the tests seemed to be working, tough. Must be because the code base is working just fine 👍
In order to be able to clean up without having to do everything by hand, requiring deeper knowledge of the actual test cases, rimraf was added as a dev dependency.
@raszi I am also considering to use it for async recursive dir/file removal, and also for sync dir/file removal. that way we could eliminate a lot of code from tmp. What do you think? rimraf seems to be stable and also provides a work around for weird win32 platform related issues.